Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[16143] Implement Convert Step Updates #16710

Merged
merged 17 commits into from
Dec 13, 2024
Merged

[16143] Implement Convert Step Updates #16710

merged 17 commits into from
Dec 13, 2024

Conversation

wcutshall
Copy link
Collaborator

@wcutshall wcutshall commented Dec 4, 2024

This PR contains coding changes for user story 16144.

Test Steps:

  1. Debug unit tests or integration tests locally with breakpoints set in affected code to verify functionality.
  2. POST a message through Postman (or equivalent) locally and observe the log files.
  3. Connect dev env to testing app insights and verify events show up properly.

Changes

  • Modified FHIRConverter and associated tests to:
    • Only add extensions if message is an ELR.
    • Send an ITEM_TRANSFORMED Azure event.

Checklist

Testing

  • Tested locally?
  • Ran ./prime test or ./gradlew testSmoke against local Docker ReportStream container?
  • Added tests?

Process

No changes.

Copy link
Contributor

github-actions bot commented Dec 4, 2024

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Manifest Files

Copy link
Contributor

github-actions bot commented Dec 4, 2024

Test Results

1 267 tests  +9   1 263 ✅ +9   7m 31s ⏱️ -31s
  164 suites ±0       4 💤 ±0 
  164 files   ±0       0 ❌ ±0 

Results for commit 5ccea0d. ± Comparison against base commit 6cb62ec.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Dec 4, 2024

Integration Test Results

 60 files  ±0   60 suites  ±0   36m 18s ⏱️ - 2m 12s
424 tests ±0  414 ✅ ±0  10 💤 ±0  0 ❌ ±0 
427 runs  ±0  417 ✅ ±0  10 💤 ±0  0 ❌ ±0 

Results for commit 5ccea0d. ± Comparison against base commit 6cb62ec.

♻️ This comment has been updated with latest results.

@wcutshall wcutshall marked this pull request as ready for review December 10, 2024 17:08
@wcutshall wcutshall requested a review from a team as a code owner December 10, 2024 17:08
@@ -323,6 +326,10 @@ class FHIRConverter(
// We know from the null check above that this cannot be null
val bundle = processedItem.bundle!!
transformer?.process(bundle)
logger.info(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions to make this log message more detailed:

  1. Do this log after you make the child report (line 335) so that you can set childReportId to an actual value
  2. (optional) You can access the processedItem var here, so add the tracking id of the item to this log message: processedItem.getTrackingId().

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Changes made.

@@ -508,24 +514,32 @@ class FHIRConverter(
}
// 'stamp' observations with their condition code
if (item.bundle != null) {
val isElr = if (item.bundle!!.getRSMessageType() == RSMessageType.LAB_RESULT) true else false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified to: item.bundle!!.getRSMessageType() == RSMessageType.LAB_RESULT

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup.

UnmappableConditionMessage(
it.failures.map { it.code },
it.source
// Only do this if it is an ELR item.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment not needed, the code is clear enough. Try to use comments to explain the "why" not the "what". In this case, no comment is needed imo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaned it up.

}
}
}
logger.info(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this exact log message again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This came out of the conversation we had when we were reviewing the code previously. The gist of the conversation was that you thought we should capture every time the message was modified in some way.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we already have a log for when a transform is applied? FHIRConverter line 342

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

*
* @return true if has a MesssageHeader that contains an R01 or ORU_R01, otherwise false.
*/
fun Bundle.isElr(): Boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we tried moving the logic here into getRSMessageType? I think that would be cleaner, mostly because the code in this method that reads the message header value is going to be reused by test orders as well as other message types probably, right? I don't see the value of an isELR method.

Copy link
Collaborator Author

@wcutshall wcutshall Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought on this is that, if we intend to add several potential groupings (i.e., enums in RSMessageType), then getRSMessageType would become quite large. Keeping just meaningful function names related to the message grouping type makes the switch statement clearer in my opinion. Additionally, there is an old programming rule of thumb that goes something like "no subroutine should be longer than one screen in length". This, obviously, is rather vague but speaks to keeping code segments small to increase "understandability". You'll often see this argument backed by references like The Magic Number Seven, Plus or Minus Two.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I just think in practice in this case this won't be longer than a page etc and the downside is you'll be copy pasting this function most likely and just changing the if check in it (violating the DRY principal). That said, there is no repeated code right now, so I'll leave this as a "nit" and we can refactor if needed when we expand if you'd like.

*/
fun Bundle.getRSMessageType(): RSMessageType {
when {
isElr() -> return RSMessageType.LAB_RESULT
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you update this method per my suggestion above, you can just do this instead:

event is Coding && ((event.code == "R01") || (event.code == "ORU_R01")) -> return RSMessageType.LAB_RESULT

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment about your suggestion above.

@@ -544,6 +646,12 @@ class FhirConverterTests {
}
}

// @Test
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please delete

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

* @return RSMessageType of this Bundle.
*/
fun Bundle.getRSMessageType(): RSMessageType {
when {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kotlin tip: when expressions return values so you can omit your returns in each branch and return the whole expression.

return when {
    isElr() -> RSMessageType.LAB_RESULT
    else -> RSMessageType.UNKNOWN
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Gradle build fails when your suggested change is made.

> Task :prime-router:compileKotlin FAILED
e: file:///Users/bill/projects/report-stream/prime-reportstream/prime-router/src/main/kotlin/fhirengine/utils/FHIRBundleHelpers.kt:151:1 A 'return' expression required in a function with a block body ('{...}')

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops my mistake, apologies. Made the change as requested.

.filterIsInstance<CodeType>()
.firstOrNull()
?.code
if (code != null && ((code == "R01") || (code == "ORU_R01"))) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you can just return this expression rather using the mutable isElr variable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and you can also skip the null check since null == "RO1 will be false as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noice! Made the change.

@wcutshall wcutshall merged commit 608556b into main Dec 13, 2024
25 checks passed
@wcutshall wcutshall deleted the platform/bill/16143 branch December 13, 2024 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform Platform Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants